Skip to content

Avoid a crash on method callables in generic array arguments - #6701

Open
drewmt wants to merge 4 commits into
phpstan:2.3.xfrom
drewmt:fix/array-method-callable
Open

drewmt wants to merge 4 commits into
phpstan:2.3.xfrom
drewmt:fix/array-method-callable

Conversation

@drewmt

@drewmt drewmt commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Fixes phpstan/phpstan#15432.

Generic array arguments containing first-class callables and closures can crash while PHPStan constructs the preliminary array type. Returning an unparameterized Closure from the initializer resolver also loses the return type used to infer a sibling closure's parameter.

Handle this in ArgumentsHandler::gatherArrayArgTypeSkeleton(), without evaluating receivers or argument expressions. Scope-known instance method callables retain their method variants through createFirstClassCallable(). Expressions that require a walk use mixed, including anonymous receivers and expression-named function or static callables. Remove the initializer fallback from the earlier revision and mirror the skeleton fix in Turbo.

The skeleton also stops reusing pre-array scope state after a potentially state-changing key or value. Keys are priced before values, including nested arrays; closure bodies are not executed. Later constants remain precise. This prevents incorrect nested closure types after assignments, increments or impure/by-reference calls. The real argument walk still replaces the skeleton before template argument observation.

Regression coverage includes the original anonymous receiver, expression-named callables, method/hoisted/static equivalence, dynamic callable names, assignments, increments, nested and key effects, by-reference calls, and stable/deferred-body controls. Existing incompatible-callable diagnostics are retained. The type assertions run with and without bleeding edge.

Local validation:

  • Full PHP 8.4 suites, without and with active Turbo: 22,476 tests, 96,910 assertions, 106 existing skips each, no failures.
  • Full PHP 8.3 suite: 22,329 tests, 96,661 assertions, 204 existing skips, no failures.
  • Both self-analysis modes, scoped coding standards and whitespace checks pass.
  • Strict native build, differential smoke tests, 16,390-member signature parity and identical whole-walk traces on the four regression fixtures (884 lines) pass.
  • Declaration parity passes on the upstream gate's PHP 8.5 runtime; no generated declarations were changed.

Cross-platform, sanitizer and downstream checks remain for the new-head upstream CI; pending checks are not local test results.

@staabm

staabm commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

please bisect which commit broke this

@drewmt

drewmt commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Bisected to 7a8c43cf265f16625a9d626d1fa6637bf505d9fa, which introduced gatherArrayArgTypeSkeleton().

Its parent dfe44f6c7 analyses the method-callable regression without errors; that commit throws ShouldNotHappenException from getFirstClassCallableType(). I confirmed both with their identical committed dependency lockfile. The new skeleton path passes an unwalked MethodCall to the initializer resolver, where that first-class-callable case was missing.

@drewmt
drewmt force-pushed the fix/array-method-callable branch from 66352a3 to 8a2ca96 Compare October 8, 2026 17:32
@drewmt

drewmt commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current 2.3.x to resolve the Turbo pin conflict; the implementation is unchanged. Full suites pass with and without Turbo (22,459 tests each), and the rebuilt native version and parity checks pass. New-head CI is running.

@ondrejmirtes ondrejmirtes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The returned new ObjectType(Closure::class) is not good enough. Because it ends up in templateArgumentObserver->collectArgument in processArgs, it influences bidirectional type narrowing results.

Here's a failing test on top of your branch. Add it to your code and fix that.

@ondrejmirtes

Copy link
Copy Markdown
Member

Thanks! The crash is fixed for the reported snippet, but I don't think returning Closure for MethodCall is the right fix. gatherArrayArgTypeSkeleton() asks InitializerExprTypeResolver about expressions that aren't constant expressions, and this PR only covers one of them.

1. Other first-class callables still crash on this branch

getFirstClassCallableType() still throws for a FuncCall whose name is an expression:

/**
 * @template T
 * @param array{producer: callable(): T, consumer: callable(T): void} $pair
 */
function pipe(array $pair): void {}

function a(\Closure $f): void
{
	pipe(['producer' => $f(...), 'consumer' => function ($v): void {}]);        // Internal error
	pipe(['producer' => 'strlen'(...), 'consumer' => function ($v): void {}]);  // Internal error
}

2. A method callable gives a less precise type than its equivalent forms

The skeleton exists to type the closures nested in the array. A plain Closure leaf resolves T to mixed. Every other way of writing the same callable resolves it to int:

class Producer
{
	public function produce(): int { return 1; }
	public static function produceStatic(): int { return 1; }
}

function test(Producer $p): void
{
	pipe(['producer' => $p->produce(...), 'consumer' => function ($v): void {
		\PHPStan\dumpType($v); // mixed
	}]);

	$callable = $p->produce(...);
	pipe(['producer' => $callable, 'consumer' => function ($v): void {
		\PHPStan\dumpType($v); // int
	}]);

	pipe(['producer' => Producer::produceStatic(...), 'consumer' => function ($v): void {
		\PHPStan\dumpType($v); // int
	}]);
}

I'd fix this in gatherArrayArgTypeSkeleton() itself rather than in InitializerExprTypeResolver:

  • For a MethodCall first-class callable whose receiver has a scope-known type (findScopeStateType($expr->var, $scope)), build the type from the method's variants via createFirstClassCallable().
  • For anything else that needs a walk (anonymous class receivers, $f(...), 'strlen'(...)), return mixed. That is what the skeleton's own docblock promises ("mixed for anything that needs a walk"), and it covers point 1 without a new case per node type.

3. Not caused by this PR, but it's in the same code: the skeleton reads stale scope state

Leaves are priced from the scope before the array is evaluated. A sibling that changes state earlier in the array therefore gives the nested closure a wrong type, not just a less precise one. This is a false positive in 2.3.x that 2.2.x doesn't report:

/**
 * @template T
 * @param array{first: mixed, value: T, callback: callable(T): void} $spec
 */
function run(array $spec): void {}

function fp(): void
{
	$i = 0;
	run([
		'first' => $i++,
		'value' => $i,                  // skeleton prices this as 0, at runtime it's 1
		'callback' => function ($v): void {
			if ($v === 1) {             // "Strict comparison using === between 0 and 1 will always evaluate to false."
				echo 'one';
			}
		},
	]);
}

The same happens with 'w' => $x = 'str', 'v' => $x ($v is 1, $r is 'str') and with 'w' => $this->reset(), 'v' => $this->prop ($v is still the narrowed 5). The skeleton should stop trusting scope state for every leaf that comes after the first element that can change scope (assignments, ++/--, impure calls, by-ref arguments) and use mixed for those leaves instead.

Results are the same with and without bleeding edge. The skeleton never reaches TemplateArgumentObserver::collectArgument() or observeUnpackedArgument(): the walk overwrites $gatheredArgTypeByIndex[$i] before either is called. It only affects the parameter type the array literal is walked with, so the problem is confined to the closures nested in the array.

@drewmt

drewmt commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed examples. I moved the fix into the array skeleton and removed the initializer fallback. Scope-known method callables now retain their variants, while walk-dependent callable expressions use mixed without an eager receiver walk. The skeleton also stops trusting pre-array state after potentially state-changing keys or values, including nested arrays. The new type regressions pass with and without bleeding edge; full PHP and Turbo suites pass (22,476 tests each), including the existing incompatible-callable control. New-head CI is running.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2.3.0 regression: internal error with first-class method callable and closure in a generic array argument

3 participants